Skip to content

Add callsite revalidation optout #14542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Add callsite revalidation optout #14542

wants to merge 8 commits into from

Conversation

@dadamssg
Copy link
Contributor

@dadamssg dadamssg commented Nov 13, 2025 •
edited
Loading

This adds callsite revalidation optout as brought up here: #10006

<Form method='post' defaultShouldRevalidate={false}>
fetcher.submit(target, {
  method: 'post',
  defaultShouldRevalidate: false
}

If this new value evaluates to false, it will skip calling the currently matched route's shouldRevalidate functions entirely, superseding and short-circuiting them.

@changeset-bot
Copy link

changeset-bot bot commented Nov 13, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 0715840

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
react-router Patch
@react-router/architect Patch
@react-router/cloudflare Patch
@react-router/dev Patch
react-router-dom Patch
@react-router/express Patch
@react-router/node Patch
@react-router/serve Patch
@react-router/fs-routes Patch
@react-router/remix-routes-option-adapter Patch
create-react-router Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dadamssg dadamssg force-pushed the callsite-revalidation-optout branch from 23aff25 to 3ee7f30 Compare November 13, 2025 19:30
@dadamssg dadamssg force-pushed the callsite-revalidation-optout branch from 3ee7f30 to 62e3f8f Compare November 13, 2025 20:38
@brophdawg11 brophdawg11 self-assigned this Nov 13, 2025
@dadamssg dadamssg force-pushed the callsite-revalidation-optout branch 2 times, most recently from a015f8b to 8929118 Compare November 20, 2025 15:52
@dadamssg dadamssg force-pushed the callsite-revalidation-optout branch from 8929118 to 9c211ae Compare November 20, 2025 16:16
brophdawg11
brophdawg11 reviewed Nov 20, 2025
View reviewed changes
brophdawg11
brophdawg11 reviewed Nov 20, 2025
View reviewed changes
@brophdawg11
Copy link
Contributor

brophdawg11 commented Nov 20, 2025

General approach is looking good on a quick pass! A few minor comments - I'll do some deeper testing once those are updated

@dadamssg dadamssg force-pushed the callsite-revalidation-optout branch from d879db7 to 67ee043 Compare November 20, 2025 17:17
@dadamssg
Copy link
Contributor Author

dadamssg commented Nov 20, 2025

Updated per your comments. good calls 👍

@brophdawg11
Copy link
Contributor

brophdawg11 commented Nov 21, 2025

This is looking awesome! I made some very minor updates inside the router, and instead of testing locally with an app I just wrote a bunch of tests and they all passed so I think we're good on that front. I prefixed it with unstable_ for initial release. I'll get an experimental cut or some early alpha testing in a bit here.

brophdawg11
brophdawg11 reviewed Nov 21, 2025
View reviewed changes
packages/react-router/lib/router/router.ts
Comment on lines +5061 to +5069
let defaultShouldRevalidate: boolean;
if (typeof callSiteDefaultShouldRevalidate === "boolean") {
// Use call-site value verbatim if provided
defaultShouldRevalidate = callSiteDefaultShouldRevalidate;
} else if (shouldSkipRevalidation) {
defaultShouldRevalidate = false;
} else {
defaultShouldRevalidate = isRevalidationRequired;
}
Copy link
Contributor

@brophdawg11 brophdawg11 Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only functional change - we also need to apply this to any fetcher.load revalidation checks

@brophdawg11
Copy link
Contributor

brophdawg11 commented Nov 21, 2025

I just kicked off an experimental release from this branch for some alpha testing: 0.0.0-experimental-2d8f5c807

@dadamssg
Copy link
Contributor Author

dadamssg commented Nov 21, 2025

woohoo! Thanks for carrying it to the finish line @brophdawg11!

@brookslybrand
Copy link
Contributor

brookslybrand commented Nov 21, 2025

Am I missing something or holding it wrong?

stackblitz

wat.yafw.balanced.mp4

@dadamssg
Copy link
Contributor Author

dadamssg commented Nov 22, 2025 •
edited
Loading

ooh good catch @brookslybrand. It's because you're actually exporting a shouldRevalidate in the route. We I didn't check for that 🙃 It looks like Matt did add a test explicitly for that. I'll have to dig into why the test behaves differently.

Problem is here @brophdawg11.

react-router/packages/react-router/lib/dom/ssr/routes.tsx

Lines 607 to 613 in a947b03

// Single fetch revalidates by default, so override the RR default value which
// matches the multi-fetch behavior with `true`
if (ssr && route.shouldRevalidate) {
let fn = route.shouldRevalidate;
return (opts: ShouldRevalidateFunctionArgs) =>
fn({ ...opts, defaultShouldRevalidate: true });
}

The existence of this code and the comment makes me believe it might not be as simple as just deleting though. 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brophdawg11 brophdawg11 brophdawg11 left review comments

Assignees

@brophdawg11 brophdawg11

Labels

CLA Signed feat:router pkg:react-router

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dadamssg @brophdawg11 @brookslybrand